Skip to content

fix(sf|quadbin): QUADBIN_FROMLONGLAT not clamping latitudes and return some quadbin functions return NULL when NULL parameters #456

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Dec 29, 2023
Merged

fix(sf|quadbin): QUADBIN_FROMLONGLAT not clamping latitudes and return some quadbin functions return NULL when NULL parameters #456

merged 3 commits into from
Dec 29, 2023

Conversation

vdelacruzb
Copy link
Contributor

Description

Shortcut

The issue was only detected in this function. Also started returning NULL from some SQL functions that were returning non easy to debug errors for the user.

Type of change

  • Fix

Acceptance

SELECT CARTO_DEV_DATA.carto.QUADBIN_FROMLONGLAT(-3.7038, 100, 4);
-- CRASH: 100043 (2201E): Invalid floating point operation: log(e,-11.4301)
SELECT CARTO_DATA_ENGINEERING_TEAM.vdelacruz_carto.QUADBIN_FROMLONGLAT(-3.7038, 100, 4);
-- WORKS: 5206548197333270527

-- All these functions return NULL now
SELECT CARTO_DATA_ENGINEERING_TEAM.vdelacruz_carto.QUADBIN_FROMQUADKEY(NULL);
SELECT CARTO_DATA_ENGINEERING_TEAM.vdelacruz_carto.QUADBIN_FROMZXY(NULL, 7, 6);
SELECT CARTO_DATA_ENGINEERING_TEAM.vdelacruz_carto.QUADBIN_FROMZXY(4, NULL, 6);
SELECT CARTO_DATA_ENGINEERING_TEAM.vdelacruz_carto.QUADBIN_FROMZXY(4, 7, NULL);
SELECT CARTO_DATA_ENGINEERING_TEAM.vdelacruz_carto.QUADBIN_KRING_DISTANCES(NULL, 1);
SELECT CARTO_DATA_ENGINEERING_TEAM.vdelacruz_carto.QUADBIN_KRING_DISTANCES(5207251884775047167, NULL);
SELECT CARTO_DATA_ENGINEERING_TEAM.vdelacruz_carto.QUADBIN_KRING(NULL, 1);
SELECT CARTO_DATA_ENGINEERING_TEAM.vdelacruz_carto.QUADBIN_KRING(5207251884775047167, NULL);
SELECT CARTO_DATA_ENGINEERING_TEAM.vdelacruz_carto.QUADBIN_SIBLING(NULL, 'up');
SELECT CARTO_DATA_ENGINEERING_TEAM.vdelacruz_carto.QUADBIN_SIBLING(5207251884775047167, NULL);
SELECT CARTO_DATA_ENGINEERING_TEAM.vdelacruz_carto.QUADBIN_TOPARENT(NULL, 3);
SELECT CARTO_DATA_ENGINEERING_TEAM.vdelacruz_carto.QUADBIN_TOPARENT(5207251884775047167, NULL);
SELECT CARTO_DATA_ENGINEERING_TEAM.vdelacruz_carto.QUADBIN_TOPARENT(5207251884775047167, -1);
SELECT CARTO_DATA_ENGINEERING_TEAM.vdelacruz_carto.QUADBIN_TOPARENT(5207251884775047167, 27);
SELECT CARTO_DATA_ENGINEERING_TEAM.vdelacruz_carto.QUADBIN_TOQUADKEY(NULL);
SELECT CARTO_DATA_ENGINEERING_TEAM.vdelacruz_carto.QUADBIN_TOZXY(NULL);

@vdelacruzb vdelacruzb requested a review from jgoizueta December 27, 2023 12:56
@shortcut-integration Shortcut Integration
Copy link

This pull request has been linked to Shortcut Story #322342: QUADBIN_FROMLONGLAT not clamping latitudes successfully.

@vdelacruzb
Copy link
Contributor Author

I considered adding tests, but not sure if it's worth it as this maybe should be the default behaviour.

@jgoizueta
Copy link
Contributor

The implementation is fine, and the result NULLs a big improvement.
But I don't agree with the idea of clamping (GREATEST(-85.05, LEAST(85.05, latitude))) because we altering the input point. I think that QUADBIN_FROMLONGLAT should fail with some input coordinates out of range for QUADBIN error.

@jgoizueta
Copy link
Contributor

jgoizueta commented Dec 28, 2023 •
edited
Loading

Perhaps we could add some clamping function, and even some "SAFE" function to cover different use cases:

QUADBIN_FROMLONGLAT(-3.7038, 100, 4) -- ERROR: coordinates out of valid QUADBIN range
QUADBIN_FROMCLAMPLEDLONGLAT(-3.7038, 100, 4) -- 5206548197333270527
SAFE_QUADBIN_FROMLONGLAT(-3.7038, 100, 4) -- NULL

@jgoizueta
Copy link
Contributor

Another detail: for clamping I see no problem with using an approximation to the limit value like 85.05, but for detecting the limit to return an error or NULL (if we implement the functions I propose), we should use the more exact value 85.05112877980659 (SELECT 0.5 * (4.0 * ATAN(EXP(ACOS(-1))) - ACOS(-1)) * 180.0 / ACOS(-1)). Otherwise we would reject some valid points.

@vdelacruzb
Copy link
Contributor Author

Regarding clamping is the solution implemented in other providers. But you are right maybe it's better to have two versions of this function. Or by default show an error to the user.
Don't know how worth it is to have another public function in the AT because it's supposed to be an edge case.

What do you think @ernesmb?

jgoizueta
jgoizueta approved these changes Dec 28, 2023
View reviewed changes
Copy link
Contributor

@jgoizueta jgoizueta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like clamping by default, but for consistency with the other providers we should apply it in SF as well. We'll discuss what to do next for all providers about this.

@ernesmb
Copy link

ernesmb commented Dec 29, 2023

yeah I agree with @jgoizueta 's last comment. Let's apply the same we're doing with other providers and then later think about a possible better solution in all providers

@vdelacruzb vdelacruzb merged commit 178cf92 into main Dec 29, 2023
@vdelacruzb vdelacruzb deleted the bug/sc-322342/quadbin-fromlonglat-not-clamping-latitudes branch December 29, 2023 14:11
@vdelacruzb vdelacruzb mentioned this pull request Jan 17, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jgoizueta jgoizueta jgoizueta approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vdelacruzb @jgoizueta @ernesmb